Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Status.CA support #31

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Add Status.CA support #31

merged 1 commit into from
Jul 25, 2023

Conversation

inteon
Copy link
Member

@inteon inteon commented Jun 22, 2023 •
edited
Loading

Adds the option to set the CA status field on a CertificateRequest resource (for backwards compatibility).

⚠️ This PR changes the Sign signature and is a breaking change! You will have to change your sign function to work with the new signature.

HINT: you can use cert-manager's pki.ParseSingleCertificateChainPEM(certPem) to obtain a signer.PEMBundle

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 22, 2023
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 25, 2023
@inteon inteon changed the title Add ca support Add Status.CA support Jun 26, 2023
@inteon
add support for the CA status field on CertificateRequest resources
0d76d65 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
SgtCoDFish
SgtCoDFish approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve
/hold

This makes sense to me!

My concern here (without having used issuer-lib at all) is that the changed return type for Sign would be a breaking change. I've not dug into the codebase to confirm if it is or not, but I'm guessing it could be.

If it is, I think this PR should shout about it in its description and we should shout about it in a new release of issuer-lib. We told people to expect breakage but we should make it as clear as possible what breaks and how to unbreak it!

If it's not breaking, then disregard the above 😁

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2023
@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon, SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@inteon
Copy link
Member Author

inteon commented Jul 25, 2023

/lgtm /approve /hold

This makes sense to me!

My concern here (without having used issuer-lib at all) is that the changed return type for Sign would be a breaking change. I've not dug into the codebase to confirm if it is or not, but I'm guessing it could be.

If it is, I think this PR should shout about it in its description and we should shout about it in a new release of issuer-lib. We told people to expect breakage but we should make it as clear as possible what breaks and how to unbreak it!

If it's not breaking, then disregard the above 😁

I'll create a release before and after and add the breaking change in the release description!

@inteon
Copy link
Member Author

inteon commented Jul 25, 2023

/unhold

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2023
@jetstack-bot jetstack-bot merged commit 57e6d60 into main Jul 25, 2023
@inteon inteon deleted the add_CA_support branch August 14, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SgtCoDFish SgtCoDFish SgtCoDFish approved these changes

Assignees

@SgtCoDFish SgtCoDFish

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@inteon @jetstack-bot @SgtCoDFish